-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update STR page allele size histogram to show colors based on genotype quality or other values #1649
base: main
Are you sure you want to change the base?
Conversation
9cf4e3f
to
8213175
Compare
eb494ec
to
69d9539
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Really nice work.
I left a few comments in line, one about typing, two very minor things, and one "nice one" for reuse of the your sorting hook.
browser/src/ShortTandemRepeatPage/ShortTandemRepeatAttributes.tsx
Outdated
Show resolved
Hide resolved
@rileyhgrant this latest should address all your feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing those so promptly, I just left one minor comment about two !
s being (possibly) mistakenly dropped.
After double checking that, LGTM!
repeatUnitsByClassification.pathogenic && | ||
repeatUnitsByClassification.pathogenic.length === 1 && | ||
repeatUnitsByClassification.benign && | ||
repeatUnitsByClassification.unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the !
may have been mistakenly dropped from these last two conditions:
before:
(repeatUnitsByClassification as any).pathogenic &&
(repeatUnitsByClassification as any).pathogenic.length === 1 &&
!(repeatUnitsByClassification as any).benign &&
!(repeatUnitsByClassification as any).unknown
after:
repeatUnitsByClassification.pathogenic &&
repeatUnitsByClassification.pathogenic.length === 1 &&
repeatUnitsByClassification.benign &&
repeatUnitsByClassification.unknown
Just wanted to flag this in case it was unintentional!
Major changes here are: * Instead of a single `reference_region`, STRs now have a list of `reference_regions` with a single one designated the `main_reference_region` * Allele size distributions and genotype distributions were previously represented with an attempt to represent multidimensional data with a number of nested structs, which was serviceable when there were only one or two dimensions we might want to filter on, but was getting increasingly convoluted. Since this new data expands the number of dimensions further, rather than build on the former schema and confuse things more, these distributions are now represented with a flattened list of structs each of which represents a single subset of the distribution.
c54aa88
to
a425362
Compare
Key changes: * Allele size distribution plot can now show, by use of stacked bars, breakdown of each bucket by population, quality, or sex. This also involved replacing some of our custom logic with calls to the visx family of libraries. * More options for y-scaling of allele size distribution plot. * Assorted refactoring, type specifying, and similar cleanup
a425362
to
0fb8f73
Compare
remaining TODOs: